Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds GitHub integration and refactors Atproto auth: introduces Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
shared/schemas/github.ts (1)
3-6: Consider adding non-empty string validation forownerandrepo.
v.string()accepts empty strings, which would pass validation but fail silently at the GitHub API level. AminLength(1)pipe rejects these at the schema boundary.♻️ Suggested fix
export const GitHubStarBodySchema = v.object({ - owner: v.string(), - repo: v.string(), + owner: v.pipe(v.string(), v.minLength(1)), + repo: v.pipe(v.string(), v.minLength(1)), })server/api/auth/github/session.delete.ts (1)
3-11: Consider returning a structured JSON response for consistency.Other endpoints in this PR return JSON objects (e.g.,
{ starred: false }). Returning a plain string here makes it harder for clients to handle responses uniformly. A minor inconsistency, but worth aligning.Suggested fix
- return 'GitHub disconnected' + return { disconnected: true }server/api/github/star.put.ts (1)
7-47: Significant code duplication withstar.delete.ts.This handler is nearly identical to
star.delete.ts— the only differences are the HTTP method (PUTvsDELETE), theContent-Lengthheader, and the returnedstarredboolean. Consider extracting a shared helper to reduce duplication:Sketch of shared helper
// server/utils/github-star.ts export async function toggleGitHubStar(event: H3Event, method: 'PUT' | 'DELETE') { const session = await useServerSession(event) const github = session.data.github if (!github?.accessToken) { throw createError({ statusCode: 401, message: 'GitHub account not connected.' }) } const { owner, repo } = v.parse(GitHubStarBodySchema, await readBody(event)) // ... validation, fetch, error handling ... return { starred: method === 'PUT' } }Note: the path traversal concern flagged on
star.delete.tsapplies equally here.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
app/components/Header/AccountMenu.client.vueapp/components/Header/AtprotoModal.client.vueapp/components/Header/GitHubModal.client.vueapp/composables/atproto/useAtproto.tsapp/composables/github/useGitHub.tsapp/composables/github/useGitHubStar.tsapp/pages/package/[[org]]/[name].vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonnuxt.config.tsserver/api/auth/atproto/index.get.tsserver/api/auth/atproto/session.delete.tsserver/api/auth/atproto/session.get.tsserver/api/auth/github/index.get.tsserver/api/auth/github/session.delete.tsserver/api/auth/github/session.get.tsserver/api/github/star.delete.tsserver/api/github/star.put.tsserver/api/github/starred.get.tsserver/utils/auth.tsshared/schemas/github.tsshared/types/userSession.ts
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
olivermrose
force-pushed
the
feat/gh-connection
branch
from
63ed63a to
4b6ffde
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
server/api/auth/atproto/session.delete.ts (1)
5-9:⚠️ Potential issue | 🟠 MajorUse explicit key deletion for session cleanup (Line 5).
serverSession.update({ ...: undefined })can leave stale session keys instead of removing them, which weakens sign-out cleanup.Suggested fix
- await serverSession.update({ - public: undefined, - oauthSession: undefined, - oauthState: undefined, - }) + await serverSession.update((data) => { + delete data.public + delete data.oauthSession + delete data.oauthState + })In h3 sessions (as used by Nuxt 4.3.1), does `session.update({ key: undefined })` delete the key or keep it with an undefined value? Please provide the official documentation/source behaviour for `updateSession`.
🧹 Nitpick comments (1)
lunaria/files/ta-IN.json (1)
848-848: Consider aligningconnected_asphrasing with the Atmosphere string pattern.Line 848 uses a different word order from Line 836. Keeping both in the same placeholder order will make the modal copy more consistent.
Suggested tweak
- "connected_as": "ஆக இணைக்கப்பட்டது {username}" + "connected_as": "{username} ஆக இணைக்கப்பட்டது"
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (72)
app/components/Header/AccountMenu.client.vueapp/components/Header/AtprotoModal.client.vueapp/components/Header/GitHubModal.client.vueapp/composables/atproto/useAtproto.tsapp/composables/github/useGitHub.tsapp/composables/github/useGitHubStar.tsapp/pages/package/[[org]]/[name].vuei18n/locales/ar.jsoni18n/locales/az-AZ.jsoni18n/locales/bg-BG.jsoni18n/locales/bn-IN.jsoni18n/locales/cs-CZ.jsoni18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/es.jsoni18n/locales/fr-FR.jsoni18n/locales/hi-IN.jsoni18n/locales/id-ID.jsoni18n/locales/it-IT.jsoni18n/locales/ja-JP.jsoni18n/locales/kn-IN.jsoni18n/locales/nb-NO.jsoni18n/locales/ne-NP.jsoni18n/locales/pl-PL.jsoni18n/locales/pt-BR.jsoni18n/locales/ru-RU.jsoni18n/locales/ta-IN.jsoni18n/locales/te-IN.jsoni18n/locales/uk-UA.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsonlunaria/files/ar-EG.jsonlunaria/files/az-AZ.jsonlunaria/files/bg-BG.jsonlunaria/files/bn-IN.jsonlunaria/files/cs-CZ.jsonlunaria/files/de-DE.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/es-419.jsonlunaria/files/es-ES.jsonlunaria/files/fr-FR.jsonlunaria/files/hi-IN.jsonlunaria/files/id-ID.jsonlunaria/files/it-IT.jsonlunaria/files/ja-JP.jsonlunaria/files/kn-IN.jsonlunaria/files/nb-NO.jsonlunaria/files/ne-NP.jsonlunaria/files/pl-PL.jsonlunaria/files/pt-BR.jsonlunaria/files/ru-RU.jsonlunaria/files/ta-IN.jsonlunaria/files/te-IN.jsonlunaria/files/uk-UA.jsonlunaria/files/zh-CN.jsonlunaria/files/zh-TW.jsonnuxt.config.tsserver/api/auth/atproto/index.get.tsserver/api/auth/atproto/session.delete.tsserver/api/auth/atproto/session.get.tsserver/api/auth/github/index.get.tsserver/api/auth/github/session.delete.tsserver/api/auth/github/session.get.tsserver/api/github/star.delete.tsserver/api/github/star.put.tsserver/api/github/starred.get.tsserver/utils/auth.tsserver/utils/github.tsshared/schemas/github.tsshared/types/userSession.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- shared/schemas/github.ts
- app/components/Header/GitHubModal.client.vue
- server/utils/auth.ts
- server/api/github/star.put.ts
- server/api/github/starred.get.ts
- nuxt.config.ts
- server/api/github/star.delete.ts
2 participants
🔗 Linked issue
#479
🧭 Context
Implements GitHub OAuth, displays whether a repo is starred or not, and the ability to star/unstar through the UI. This does not implement the search improvements portion.
Demo
demo.mp4
📚 Description
.envrequiresGITHUB_CLIENT_IDandGITHUB_CLIENT_SECRETfrom an OAuth app in order to run. The redirect uri needs to behttp://127.0.0.1:3000, nothttp://localhost:3000. This also needs to be the url that is visited in the browser in order for the cookie to persist properly./githubbecause I wasn't sure if a/starpath was needed/wanted.